Skip to content

TYP: Annotate groupby/ops.py #32921

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 30 commits into from
Apr 27, 2020
Merged

TYP: Annotate groupby/ops.py #32921

merged 30 commits into from
Apr 27, 2020

Conversation

dsaxton
Copy link
Member

@dsaxton dsaxton commented Mar 23, 2020

Adding a few type annotations to this file. I think the values arguments here should be ndarrays but am not positive.

@pep8speaks
Copy link

pep8speaks commented Mar 23, 2020 •
edited
Loading

Hello @dsaxton! Thanks for updating this PR. We checked the lines you've touched for PEP 8 issues, and found:

There are currently no PEP 8 issues detected in this Pull Request. Cheers! 🍻

Comment last updated at 2020-04-26 21:53:19 UTC

WillAyd
WillAyd requested changes Mar 23, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks! Generally if you can add subscripts for List / Callable would be ideal

Also the np.ndarray usage might be suspect. It resolves to Any right now so will pass the checker, but we might accept more than just ndarrays in those methods

@WillAyd WillAyd added the Typing type annotations, mypy/pyright type checking label Mar 23, 2020
dsaxton and others added 3 commits March 22, 2020 21:36
@dsaxton @WillAyd
Update pandas/core/groupby/ops.py
0e0cff3 
Co-Authored-By: William Ayd <william.ayd@icloud.com>
@dsaxton
Use ellipsis
1282d17
@dsaxton
List -> List[Index]
bc87698
@simonjayhawkins simonjayhawkins added this to the 1.1 milestone Mar 23, 2020
dsaxton
dsaxton commented Mar 23, 2020
View reviewed changes
@jreback
Copy link
Contributor

jreback commented Mar 29, 2020

merge master

jbrockmendel
jbrockmendel reviewed Apr 2, 2020
View reviewed changes
jbrockmendel
jbrockmendel reviewed Apr 2, 2020
View reviewed changes
jbrockmendel
jbrockmendel reviewed Apr 3, 2020
View reviewed changes
jbrockmendel
jbrockmendel reviewed Apr 3, 2020
View reviewed changes
jbrockmendel
jbrockmendel reviewed Apr 3, 2020
View reviewed changes
WillAyd
WillAyd reviewed Apr 4, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

one comment + Brock comment; otherwise lgtm

pandas/core/groupby/ops.py Outdated
@@ -828,11 +832,11 @@ def result_index(self):
return self.binlabels

@property
def levels(self):
def levels(self) -> List:
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think List[int] here as well

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could this perhaps be List[Index] since this is [self.binlabels]? It looks like at line 733 we call ensure_index(binlabels) and below here we call binlabels.name. The docstring is weird though ("binlabels : the label list")

jreback
jreback requested changes Apr 10, 2020
View reviewed changes
Copy link
Contributor

@jreback jreback left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can you rebase on master. just merged a change by @simonjayhawkins , see here: #33456 for callables

jbrockmendel
jbrockmendel reviewed Apr 16, 2020
View reviewed changes
pandas/core/groupby/ops.py
@@ -916,7 +918,7 @@ def _chop(self, sdata: Series, slice_obj: slice) -> Series:


class FrameSplitter(DataSplitter):
def fast_apply(self, f, sdata: FrameOrSeries, names):
def fast_apply(self, f: F, sdata: FrameOrSeries, names):
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can we say anything about names?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Possibly List[Label] but I'm not sure

@jbrockmendel
Copy link
Member

LGTM. @simonjayhawkins are we at a point where mypy would tell us if anything here were incorrect?

@simonjayhawkins
Copy link
Member

LGTM. @simonjayhawkins are we at a point where mypy would tell us if anything here were incorrect?

using the word 'incorrect' I think would only apply if we have 100% static typing. I don't think we have an appetite for that (achieving consensus in the pandas team). By 100% static typing, I mean 0% dynamic types. That means nothing resolves to Any. That includes all imports and cython code. It also includes code patterns that create classes, methods and attributes etc dynamically. We have alot of dynamic code.

Mypy tells us if the types added here are 'consistent' with the other types already added. The more types added and the more refined the types the better the 'correctness'. Again, there does not seem to be an appetite for some patterns that refine types or workflows that expedite the addition of types.

IMO with typing, 'Job done is better than perfect'. Simply put, the more types added overall, the more confidence we can get for refactoring, enhancement PRs etc.

so PRs that add types, keep mypy green and are not obviously wrong get a thumbs up from me.

partial typed function signatures are better than none. unparameterized generics are more refined than Any. If we did want to enforce this we would use --disallow-incomplete-defs and --disallow-any-generics. Mypy can do this checking so that reviewers don't need to.

WillAyd
WillAyd approved these changes Apr 22, 2020
View reviewed changes
Copy link
Member

@WillAyd WillAyd left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lgtm

@jreback
Copy link
Contributor

jreback commented Apr 26, 2020

can you merge master, ping on green.

@dsaxton
Copy link
Member Author

dsaxton commented Apr 27, 2020

can you merge master, ping on green.

@jreback Merged and green

@jreback jreback merged commit f683473 into pandas-dev:master Apr 27, 2020
rhshadrach pushed a commit to rhshadrach/pandas that referenced this pull request May 10, 2020
@dsaxton @WillAyd @rhshadrach
TYP: Annotate groupby/ops.py (pandas-dev#32921)
71608ab 
* TYP: Annotate groupby/ops.py

* Blacken

* Update pandas/core/groupby/ops.py

Co-Authored-By: William Ayd <william.ayd@icloud.com>

* Use ellipsis

* List -> List[Index]

* Specify Callable types

* More Callable subscripts

* Update

* No ArrayLike

* Import

* Update

* Use F

* Lint

Co-authored-by: William Ayd <william.ayd@icloud.com>
@dsaxton dsaxton deleted the type-ops branch March 30, 2021 20:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@jbrockmendel jbrockmendel jbrockmendel left review comments

@WillAyd WillAyd WillAyd approved these changes

@jreback jreback jreback approved these changes

Assignees
No one assigned
Labels
Typing type annotations, mypy/pyright type checking
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
6 participants
@dsaxton @pep8speaks @jreback @jbrockmendel @simonjayhawkins @WillAyd